Add new lint `manual_is_power_of_two` by Sour1emon · Pull Request #13327 · rust-lang/rust-clippy

Sour1emon · 2024-08-31T23:44:54Z

Suggest using is_power_of_two() instead of the manual implementations for some basic cases

changelog: new [manual_is_power_of_two] lint

rustbot · 2024-08-31T23:44:59Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

clippy_lints/src/manual_is_power_of_two.rs

bors · 2024-09-03T17:46:27Z

☔ The latest upstream changes (presumably #13247) made this pull request unmergeable. Please resolve the merge conflicts.

alex-semenyuk · 2024-09-05T11:02:28Z

clippy_lints/src/manual_is_power_of_two.rs

+            && let ExprKind::Lit(lit1) = right.kind
+            && let LitKind::Int(Pu128(0), _) = lit1.node
+        {
+            let snippet = snippet_with_applicability(cx, left1.span, "..", &mut applicability);


Might make sense combine sugg and span_lint_and_sugg here and 48-58 so change in one place if needed

alex-semenyuk · 2024-09-05T11:10:20Z

tests/ui/manual_is_power_of_two.fixed

+
+    let b = 4_i64;
+
+    // is_power_of_two only works for unsigned integers


Might be good idea mentioned about this at lint doc as well

llogiq

There is some duplication and the binary operations might be reversed. Either you implement the tests for both directions or you document the missing cases in the lint docs under # Known Problems and we can open an issue to amend the lint later. Both are fine.

clippy_lints/src/manual_is_power_of_two.rs

tests/ui/manual_is_power_of_two.rs

bors · 2024-09-05T23:29:28Z

☔ The latest upstream changes (presumably #12987) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot · 2024-09-06T01:24:34Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits:

a29f0c4

Co-authored-by: Ruby Lazuli <general@patchmixolydic.com>

llogiq · 2024-09-06T10:33:39Z

clippy_lints/src/manual_is_power_of_two.rs

+            if let ExprKind::MethodCall(method_name, reciever, _, _) = left.kind
+                && method_name.ident.as_str() == "count_ones"
+                && let &Uint(_) = cx.typeck_results().expr_ty(reciever).kind()
+                && check_lit(right, 1)
+            {
+                build_sugg(cx, expr, reciever, &mut applicability);
+            }
+
+            // 1 == a.count_ones()
+            if let ExprKind::MethodCall(method_name, reciever, _, _) = right.kind
+                && method_name.ident.as_str() == "count_ones"
+                && let &Uint(_) = cx.typeck_results().expr_ty(reciever).kind()
+                && check_lit(left, 1)
+            {
+                build_sugg(cx, expr, reciever, &mut applicability);
+            }


You can loop over [(left, right), (right, left)] to reduce the code duplication here and elsewhere.

llogiq · 2024-09-06T10:35:50Z

Apart from a good deal of code duplication, this seems good to merge, we can refactor in a followup PR later.

@bors r+

bors · 2024-09-06T10:35:52Z

📌 Commit f994797 has been approved by llogiq

It is now in the queue for this repository.

bors · 2024-09-06T10:37:00Z

⌛ Testing commit f994797 with merge fb9913e...

bors · 2024-09-06T10:48:45Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing fb9913e to master...

rustbot assigned llogiq Aug 31, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 31, 2024

Alexendoo mentioned this pull request Sep 1, 2024

Add lints against more manual integer ops where direct methods exist #12894

Open

10 tasks

PatchMixolydic reviewed Sep 2, 2024

View reviewed changes

clippy_lints/src/manual_is_power_of_two.rs Outdated Show resolved Hide resolved

clippy_lints/src/manual_is_power_of_two.rs Outdated Show resolved Hide resolved

alex-semenyuk reviewed Sep 5, 2024

View reviewed changes

llogiq reviewed Sep 5, 2024

View reviewed changes

clippy_lints/src/manual_is_power_of_two.rs Outdated Show resolved Hide resolved

tests/ui/manual_is_power_of_two.rs Show resolved Hide resolved

rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Sep 6, 2024

rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Sep 6, 2024

Sour1emon and others added 4 commits September 5, 2024 18:38

Add manual_is_power_of_two

d7996da

Improve "Why this is bad" section

5515566

Co-authored-by: Ruby Lazuli <general@patchmixolydic.com>

Fixed extra backtick

6ecb48f

Co-authored-by: Ruby Lazuli <general@patchmixolydic.com>

Add support for different orders of expression

f994797

llogiq reviewed Sep 6, 2024

View reviewed changes

bors merged commit fb9913e into rust-lang:master Sep 6, 2024

Sour1emon deleted the master branch September 7, 2024 17:38

xFrednet mentioned this pull request Oct 16, 2024

Change the category of manual_is_power_of_two to pedantic #13553

Merged


		let b = 4_i64;

		// is_power_of_two only works for unsigned integers

Conversation

Sour1emon commented Aug 31, 2024 • edited by llogiq Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Aug 31, 2024

Uh oh!

Uh oh!

Uh oh!

bors commented Sep 3, 2024

Uh oh!

alex-semenyuk Sep 5, 2024

Choose a reason for hiding this comment

Uh oh!

alex-semenyuk Sep 5, 2024

Choose a reason for hiding this comment

Uh oh!

llogiq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bors commented Sep 5, 2024

Uh oh!

rustbot commented Sep 6, 2024

Uh oh!

llogiq Sep 6, 2024

Choose a reason for hiding this comment

Uh oh!

llogiq commented Sep 6, 2024

Uh oh!

bors commented Sep 6, 2024

Uh oh!

bors commented Sep 6, 2024

Uh oh!

bors commented Sep 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Sour1emon commented Aug 31, 2024 •

edited by llogiq

Loading